JSpecify: infer generic method type arguments for assignments#1131
JSpecify: infer generic method type arguments for assignments#1131yuxincs merged 93 commits intouber:masterfrom
Conversation
|
@haewiful thanks for this! It looks like CI fails on the https://github.com/uber/NullAway/actions/runs/12978381939/job/36192901792?pr=1131#step:5:606 I suggest looking at the code it is crashing on and trying to write a unit test that causes the same failure, which will make it easier to debug. |
# Conflicts: # nullaway/src/main/java/com/uber/nullaway/NullAway.java
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1131 +/- ##
=========================================
Coverage 88.16% 88.16%
- Complexity 2281 2300 +19
=========================================
Files 87 88 +1
Lines 7461 7531 +70
Branches 1491 1503 +12
=========================================
+ Hits 6578 6640 +62
- Misses 445 448 +3
- Partials 438 443 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| new GenericsChecks() | ||
| .getGenericReturnNullnessAtInvocation( | ||
| ASTHelpers.getSymbol(tree), tree, state, config); |
There was a problem hiding this comment.
Hrm, this seems suspect. If you create a new GenericsChecks object here, then the inferred types will immediately disappear. Do we need to instead somehow pass in the GenericsChecks object from the NullAway instance into this class?
There was a problem hiding this comment.
Yes, I do think the GenericsChecks object should be passed from NullAway because getGenericReturnNullnessAtInvocation() method call uses what is in inferred types but doesn't add to it. If the returned type contains some type variable in the inferredTypes, it should be used here.
In my examination, the AccessPathNullnessPropagation class is used in AccessPathNullnessAnalysis which is used in the NullAway file. I think we should pass the GenericsChecks instance from NullAway to AccessPathNullnessAnalysis to AccessPathNullnessPropagation.
… AccedssPathNullnessPropagation
| if (typeVarHasNullableUpperBound) { // can just use the lhs type nullability | ||
| genericNullness.put(typeVar, lhsType); | ||
| } else { // rhs can't be nullable, use upperbound | ||
| // this is a bit weird. the nullability might be right, but the base type may be wrong? |
There was a problem hiding this comment.
@haewiful this logic here is incorrect. We shouldn't put the upper bound in the map; we should put lhsType, except that if lhsType is @Nullable, we should strip that annotation off of it. (I think you can do that by calling stripMetadata().) See the new test inferNestedNonNullUpperBound() that I added; we should get an error there, but we do not right now.
msridhar
changed the title
msridhar
changed the title
3 participants
This PR infers nullability of generic method type arguments for the case where the result of the call is assigned to a variable, e.g.:
See the tests for (many) more examples.